Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggest using dynamic import instead of require #228

Closed
wants to merge 1 commit into from

Conversation

t6adev
Copy link

@t6adev t6adev commented Jan 23, 2023

Hi, I just notice that here is used require().
Using import() instead is helpful in the doc.
Thank you.

@vercel
Copy link

vercel bot commented Jan 23, 2023

@t6adev is attempting to deploy a commit to the MSW Team on Vercel.

A member of the Team first needs to authorize it.

@@ -77,6 +77,13 @@ import App from './App'
if (process.env.NODE_ENV === 'development') {
const { worker } = require('./mocks/browser')
worker.start()
/*
// or you can do this instead
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that dynamic import works in a different way than require does, so they are not interchangeable in any regard.

  • require is not statically analyzable so bundlers cannot tree-shake its calls / import can be tree-shaken.
  • require is sync; import is async and must be awaited.

Those are very distinct differences and they directly affect how you should treat setups that utilize either method.

On a related note, we're discussing an issue in Next where the usage of dynamic import results in a race condition between the worker activation and requests on the initial load (see vercel/next.js#43284).

Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @t6adev. Thanks for proposing this. I've left a comment describing my concern with this suggestion.

@t6adev
Copy link
Author

t6adev commented Jan 24, 2023

Thank you for your quick response.
I see, I didn't care about the race condition. And I missed the note below the code (also about "Deferred mounting" page )
My idea coming up while reading your comment is already described there 😓

Ok, now this PR is able to close.
I wonder if it can be helpful if there are some comments about the race condition and tree-shaking inside the example code block. I'm following your decision. Anyway, thank you again!

@t6adev t6adev closed this Apr 13, 2023
@t6adev t6adev deleted the patch-1 branch April 13, 2023 12:11
@kettanaito
Copy link
Member

There may be an alternative route I'm experimenting with. With the Server Components in React, you can technically apply the interception and defer the client-side rendering even when using imports because async components are a thing now. Anybody reading this is free to experiment and share their findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants